Skip to content

[None][fix] Revert #18445 - #18736

Merged
tburt-nv merged 1 commit into
mainfrom
revert-18445-feat/perf-sanity-e2e-time-breakdown
Sep 4, 2026
Merged

[None][fix] Revert #18445#18736
tburt-nv merged 1 commit into
mainfrom
revert-18445-feat/perf-sanity-e2e-time-breakdown

Conversation

@tburt-nv

@tburt-nv tburt-nv commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Reverts #18445

Dev Engineer Review

  • Reverts per-request disaggregated lifecycle span uploads and removes related time-breakdown APIs, metrics, CLI options, configuration, and documentation.
  • Updates performance test-ID parsing and generation consistently across CI, local submission, and integration tests.
  • Limits device-step-time metrics to gen_only runs and removes obsolete lifecycle metric processing.
  • Reduces affected GB300 configurations from three shards to two shards.
  • Configuration and test-list changes use consistent, valid test IDs.
  • Verdict: sufficient.

QA Engineer Review

  • Modified test code:
    • Removed lifecycle span and time-breakdown coverage from test_request_metrics.py.
    • Removed time-breakdown metric tests and obsolete parser tests.
    • Updated perf sanity helper tests for unbounded log reads and gen_only metrics.
    • Updated benchmark-mode parser coverage.
    • Removed obsolete perf test-ID grammar tests.
  • Test-list changes:
    • Removed test_request_metrics.py from l0_cpu.yml.
    • Removed time-breakdown performance cases from the GB300 perf sanity lists.
    • Removed the related waiver from waives.txt.
  • The removed performance cases are no longer generated or supported by the test code. The remaining parser and metric coverage matches the reverted behavior.
  • Verdict: sufficient.

@tburt-nv
tburt-nv requested review from a team as code owners September 4, 2026 22:14
@tburt-nv tburt-nv changed the title Revert "[None][feat] perf-sanity: upload per-request disagg lifecycle spans to OpenSearch" [None][fix] Revert #18445 Sep 4, 2026
@tburt-nv
tburt-nv merged commit c4422ce into main Sep 4, 2026
12 of 15 checks passed
@tburt-nv
tburt-nv deleted the revert-18445-feat/perf-sanity-e2e-time-breakdown branch September 4, 2026 22:17
@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ede8b7b3-9849-40b3-840a-d485efeb3cb2

📥 Commits

Reviewing files that changed from the base of the PR and between c7da76e and e6ceabf.

📒 Files selected for processing (27)
  • jenkins/L0_Test.groovy
  • jenkins/scripts/perf/README.md
  • jenkins/scripts/perf/local/README.md
  • jenkins/scripts/perf/local/configs/example.conf
  • jenkins/scripts/perf/local/submit.py
  • jenkins/scripts/perf/submit.py
  • tensorrt_llm/serve/perf_metrics.py
  • tensorrt_llm/serve/scripts/benchmark_serving.py
  • tensorrt_llm/serve/scripts/time_breakdown/time_breakdown.py
  • tests/integration/defs/perf/README_test_perf_sanity.md
  • tests/integration/defs/perf/test_perf_sanity.py
  • tests/integration/defs/perf/time_breakdown_metrics.py
  • tests/integration/test_lists/test-db/l0_cpu.yml
  • tests/integration/test_lists/test-db/l0_gb300_multi_gpus_perf_sanity.yml
  • tests/integration/test_lists/test-db/l0_gb300_multi_nodes_perf_sanity_ctx12_node1_gpu4_gen1_node2_gpu8.yml
  • tests/integration/test_lists/test-db/l0_gb300_multi_nodes_perf_sanity_ctx1_node1_gpu4_gen4_node2_gpu8.yml
  • tests/integration/test_lists/test-db/l0_gb300_multi_nodes_perf_sanity_ctx3_node1_gpu4_gen1_node8_gpu32.yml
  • tests/integration/test_lists/test-db/l0_gb300_multi_nodes_perf_sanity_ctx6_node1_gpu4_gen1_node4_gpu16.yml
  • tests/integration/test_lists/waives.txt
  • tests/scripts/perf-sanity/cache_transceiver_precheck/run_precheck.py
  • tests/unittest/llmapi/apps/test_request_metrics.py
  • tests/unittest/others/test_cache_transceiver_precheck_config.py
  • tests/unittest/others/test_perf_sanity_time_breakdown.py
  • tests/unittest/others/test_time_breakdown.py
  • tests/unittest/others/test_time_breakdown_metrics.py
  • tests/unittest/scripts/test_perf_sanity_helpers.py
  • tests/unittest/scripts/test_perf_submit.py

Walkthrough

The change removes time-breakdown support from performance launchers, serving metrics, PerfSanity collection, tests, and CI configuration. Test IDs now use standard modes, and device-step-time metrics apply only to gen_only runs.

Changes

PerfSanity and timing cleanup

Layer / File(s) Summary
Simplified test-ID and launch contract
jenkins/scripts/perf/..., tests/scripts/perf-sanity/..., tests/unittest/scripts/test_perf_submit.py, tests/unittest/others/test_cache_transceiver_precheck_config.py
Launchers and configuration documentation remove time-breakdown modifiers and use four-value parser results.
Gen-only metric collection and test flow
tests/integration/defs/perf/..., tests/unittest/scripts/test_perf_sanity_helpers.py
Lifecycle aggregation is removed. Device-step-time metrics are limited to gen_only, and log parsing reads from client offsets through EOF.
Serving timing and JSONL processing
tensorrt_llm/serve/perf_metrics.py, tensorrt_llm/serve/scripts/..., tests/unittest/llmapi/apps/test_request_metrics.py
Timing headers and JSONL processing no longer support server or KV-transfer timestamps, in-memory statistics APIs, or statistics JSON export.
CI configuration and suite alignment
jenkins/L0_Test.groovy, tests/integration/test_lists/...
GB300 shard counts and performance test lists no longer include removed time-breakdown cases.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested reviewers: chenfeiz0326

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert-18445-feat/perf-sanity-e2e-time-breakdown

Comment @coderabbitai help to get the list of available commands.

chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Sep 10, 2026
…erf harness

Per maintainer direction that a perf test script does not need a unit
test suite of its own, this drops the perf-sanity unit tests that this
PR introduced:

  deleted   tests/unittest/others/test_perf_sanity_time_breakdown.py
  deleted   tests/unittest/others/test_time_breakdown_metrics.py
  reverted  tests/unittest/scripts/test_perf_sanity_helpers.py
  reverted  tests/unittest/scripts/test_perf_submit.py
  reverted  tests/unittest/others/test_cache_transceiver_precheck_config.py

The three reverted files are now byte-identical to main. Their restored
assertions were re-run against this PR's modified submit.py,
run_precheck.py and test_perf_sanity.py to confirm none of them encode
pre-change behaviour: 82, 55 and 19 passed respectively.

Pre-existing perf-sanity suites owned by other changes (NVIDIA#18408, NVIDIA#18432)
are left untouched, keeping this PR to a single concern.

Two things are retained on purpose:

  * tests/unittest/tools/test_perf_sanity_matching.py -- the __path__
    change here is the actual fix for the ModuleNotFoundError that
    caused the original revert (NVIDIA#18736), not a new test. It remains
    load-bearing after every deletion above: reverting just that hunk
    reproduces "No module named 'defs.perf.time_breakdown_metrics'"
    with 8 failures. It also keeps a real (non-stubbed) import of
    time_breakdown_metrics, so a future stdlib-only sibling cannot
    silently break the harness again.

  * test_time_breakdown.py and test_request_metrics.py -- these cover
    shipped tensorrt_llm/serve code, not the perf harness.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Sep 10, 2026
test_perf_sanity_matching.py is a perf-sanity harness test: it loads
tests/integration/defs/perf/test_perf_sanity.py by file path and imports
test_common.perf_sanity_matching, the harness's own case-matching logic.
Per maintainer direction that a perf test script does not need a unit
test suite, it is removed.

This also removes the __path__ change that this PR previously carried in
that file. That is consistent rather than a regression: the
ModuleNotFoundError which caused the original revert (NVIDIA#18736) was raised
by this file's own stub loader, so deleting the file removes the failure
at its source. No other unit test is exposed to it --
test_perf_regression_branch.py uses the same empty-__path__ stub pattern
but loads perf_regression_utils.py, which imports only
open_search_db_utils, and test_perf_sanity_helpers.py imports
defs.perf.test_perf_sanity through the real package path.

Two dangling references are cleaned up with it:

  * tests/integration/test_lists/test-db/l0_a10.yml -- the entry had to
    go. That stage runs one pytest invocation per entry, so an entry
    naming a missing file collects nothing and errors. Note that
    scripts/check_test_list.py --validate does not catch this: it still
    reports "OK" with a deliberately bogus filename injected, so the
    removal was verified by reading the stage semantics, not by the
    validator.

  * tests/integration/defs/.test_durations -- stale 23.07s key for the
    deleted test. Inert (the file is regenerated by UpdateTestDurations),
    but it pointed at a file that no longer exists. JSON re-validated,
    1603 entries.

tests/unittest/others/test_time_breakdown.py is deliberately NOT removed:
it imports only tensorrt_llm.serve.scripts.benchmark_serving and
tensorrt_llm.serve.scripts.time_breakdown, so it covers code that ships
in the wheel. Its 'time_breakdown_metrics' occurrences are dict keys in
JSON fixtures, not the defs.perf.time_breakdown_metrics module.

Remaining suites re-run after the deletion: test_perf_regression_branch
19 passed, test_perf_submit 82 passed, test_cache_transceiver_precheck_config
55 passed.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Sep 11, 2026
…o OpenSearch

Re-lands NVIDIA#18445, reverted in NVIDIA#18736 after post-merge CI hit:

  tests/unittest/tools/test_perf_sanity_matching.py::
  test_warmup_is_derived_from_exactly_the_e2e_and_ctx_only_modes
  ModuleNotFoundError: No module named 'defs.perf.time_breakdown_metrics'

The root cause is a semantic merge conflict between two independently-green
PRs, not a defect in either. NVIDIA#18432, merged two days earlier, added a
_load_module() helper that execs tests/integration/defs/perf/test_perf_sanity.py
by file path under a synthetic `defs.perf` package with an empty __path__, plus
a closed allowlist of stub siblings in sys.modules so the heavy ones (torch, the
OpenSearch client) are never imported. NVIDIA#18445 added a new real sibling,
time_breakdown_metrics, and imported it from test_perf_sanity.py. With an empty
__path__ and no stub entry, that import cannot resolve. The failing test is not
one of NVIDIA#18445's own; dropping those would leave the failure unchanged.

The fix belongs in the helper rather than in the perf code: give the synthetic
package a real __path__ so a stdlib-only sibling resolves for real.
time_breakdown_metrics is deliberately stdlib-only, so it imports cleanly in a
CPU-only job. Entries already in sys.modules still take precedence, so the heavy
siblings stay stubbed, and the next stdlib-only sibling will not break this file.

Everything else is NVIDIA#18445 unchanged, apart from two conflicts with main:

* l0_gb300_multi_gpus_perf_sanity.yml keeps main's 120-minute budget for the
  con4301 ctx_only case (NVIDIA#18859, nvbugs/6682113) and re-adds the time_breakdown
  con666 lane at 90, the budget its own workload carries.
* waives.txt is left as main has it. nvbugs/6661856 has since been unwaived and
  the con8 base cases now run, so the time_breakdown sibling is not re-waived.

Verified on CPU: test_perf_sanity_matching.py 19/19 (previously 1 failed),
test_time_breakdown_metrics.py 20/20, test_perf_submit.py 101 passed and 1
skipped (needs torch), test_cache_transceiver_precheck_config.py 60/60, and
test_perf_regression_branch.py 19/19 as an unaffected control.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Sep 11, 2026
…erf harness

Per maintainer direction that a perf test script does not need a unit
test suite of its own, this drops the perf-sanity unit tests that this
PR introduced:

  deleted   tests/unittest/others/test_perf_sanity_time_breakdown.py
  deleted   tests/unittest/others/test_time_breakdown_metrics.py
  reverted  tests/unittest/scripts/test_perf_sanity_helpers.py
  reverted  tests/unittest/scripts/test_perf_submit.py
  reverted  tests/unittest/others/test_cache_transceiver_precheck_config.py

The three reverted files are now byte-identical to main. Their restored
assertions were re-run against this PR's modified submit.py,
run_precheck.py and test_perf_sanity.py to confirm none of them encode
pre-change behaviour: 82, 55 and 19 passed respectively.

Pre-existing perf-sanity suites owned by other changes (NVIDIA#18408, NVIDIA#18432)
are left untouched, keeping this PR to a single concern.

Two things are retained on purpose:

  * tests/unittest/tools/test_perf_sanity_matching.py -- the __path__
    change here is the actual fix for the ModuleNotFoundError that
    caused the original revert (NVIDIA#18736), not a new test. It remains
    load-bearing after every deletion above: reverting just that hunk
    reproduces "No module named 'defs.perf.time_breakdown_metrics'"
    with 8 failures. It also keeps a real (non-stubbed) import of
    time_breakdown_metrics, so a future stdlib-only sibling cannot
    silently break the harness again.

  * test_time_breakdown.py and test_request_metrics.py -- these cover
    shipped tensorrt_llm/serve code, not the perf harness.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
chenfeiz0326 added a commit to chenfeiz0326/TensorRT-LLM that referenced this pull request Sep 11, 2026
test_perf_sanity_matching.py is a perf-sanity harness test: it loads
tests/integration/defs/perf/test_perf_sanity.py by file path and imports
test_common.perf_sanity_matching, the harness's own case-matching logic.
Per maintainer direction that a perf test script does not need a unit
test suite, it is removed.

This also removes the __path__ change that this PR previously carried in
that file. That is consistent rather than a regression: the
ModuleNotFoundError which caused the original revert (NVIDIA#18736) was raised
by this file's own stub loader, so deleting the file removes the failure
at its source. No other unit test is exposed to it --
test_perf_regression_branch.py uses the same empty-__path__ stub pattern
but loads perf_regression_utils.py, which imports only
open_search_db_utils, and test_perf_sanity_helpers.py imports
defs.perf.test_perf_sanity through the real package path.

Two dangling references are cleaned up with it:

  * tests/integration/test_lists/test-db/l0_a10.yml -- the entry had to
    go. That stage runs one pytest invocation per entry, so an entry
    naming a missing file collects nothing and errors. Note that
    scripts/check_test_list.py --validate does not catch this: it still
    reports "OK" with a deliberately bogus filename injected, so the
    removal was verified by reading the stage semantics, not by the
    validator.

  * tests/integration/defs/.test_durations -- stale 23.07s key for the
    deleted test. Inert (the file is regenerated by UpdateTestDurations),
    but it pointed at a file that no longer exists. JSON re-validated,
    1603 entries.

tests/unittest/others/test_time_breakdown.py is deliberately NOT removed:
it imports only tensorrt_llm.serve.scripts.benchmark_serving and
tensorrt_llm.serve.scripts.time_breakdown, so it covers code that ships
in the wheel. Its 'time_breakdown_metrics' occurrences are dict keys in
JSON fixtures, not the defs.perf.time_breakdown_metrics module.

Remaining suites re-run after the deletion: test_perf_regression_branch
19 passed, test_perf_submit 82 passed, test_cache_transceiver_precheck_config
55 passed.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: chenfeiz0326 <203214996+chenfeiz0326@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant